The attached testcase causes a crash near null in mozilla-central rev 20094311ab5e ==32475==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000030 (pc 0x7fd6ac59708f bp 0x7fffb14c63f0 sp 0x7fffb14c63b0 T0) #0 0x7fd6ac59708e in Get /home/worker/workspace/build/src/obj-firefox/dist/include/PLDHashTable.h:228:26 #1 0x7fd6ac59708e in PLDHashTable::Add(void const*, mozilla::fallible_t const&) /home/worker/workspace/build/src/xpcom/glue/PLDHashTable.cpp:535 #2 0x7fd6aeb65ae5 in PutEntry /home/worker/workspace/build/src/obj-firefox/dist/include/nsTHashtable.h:161:36 #3 0x7fd6aeb65ae5 in Put /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:150 #4 0x7fd6aeb65ae5 in Put /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:142 #5 0x7fd6aeb65ae5 in AddValue /home/worker/workspace/build/src/dom/animation/AnimValuesStyleRule.cpp:81

Oh, there is still another case. Thank you, Jesse!

I have no idea other than fixing bug 1324966. In case of the test case in comment 0, resolving style context for the base values needs flushing styles (because the target element does not have primary frame yet.), as a result, AnimValuesStyleRule is nullified at EffectCompositor::ComposeAnimationRule().

This patch adds nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation() that returns nsStyleContext without composing any animation styles. (Whereas my intention in bug 1324966 is to return nsStyleContext without composing style for *a given nsCSSPropertyID*) This patch includes lots of duplicated codes, but anyway it fixes this crash and bug 1322291 too.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > This patch includes lots of duplicated codes, but anyway it fixes this crash > and bug 1322291 too. Wrong. The patch just fixed this crash, does not fix bug 1322291 at all.

It turns out that we also need to skip UpdateEffectProperties() in nsStyleSet::GetContext().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e0b3a82ff3404bfa8dc382669732fa1f8e53e17 Here is a try with patches that adds nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation() but does not skip UpdateEffectProperties().

A new try with skipping UpdateEffectProperties() (i.e. calling GetContext() with eNoFlags instead of eDoAnimation). https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb7cdd4c273733997141245fa1ea3651677139ce

The previous patches have some mistakes. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c54d12c078a72e9b7bd944db437e234d6895ed4e

Cameron, would you mind reviewing some of these patches? Background of this issue: We are using nsCompuedDOMStyle::GetStyleContextForElement() to get nsStyleContext for gettting base style values (i.e. style values without animating style). It is processed during composing animating style (KeyframeEffectReadOnly::ComposeStyle) so that it currently causes recursive calls of KeyframeEffectReadOnly::ComposeStyle. Also, when there is an animation specified in font-size unit (e.g. em) and an animation of font-size, this recursive call overrides KeyframeEffectReadOnly.mProperties in KeyframeEffectReadOnly::UpdateProperties (called from EffectCompositor::UpdateEffectProperties) while we are processing mProperties in ComposeStyle. To prevent these symptoms we need a new function that skips all animation rules and procedure in 'if (aFlags & eDoAnimation)' block in nsStyleSet::GetContext(). The new function is GetStyleContextForElementWithoutAnimation in part 1 patch.

Comment on attachment 8826991 [details] Bug 1330190 - Part 5: Get style context without all of animation data for base styles. https://reviewboard.mozilla.org/r/104792/#review105516 r=me with comments addressed ::: dom/animation/KeyframeEffectReadOnly.h:414 (Diff revision 1) > + // Similar to the above but ignoring animation rules. We must use this to get > + // base styles. We should explain why we must use this to get base styles. Or alternatively, just say "We use this to get base styles (which don't include animation rules)". ::: dom/animation/KeyframeEffectReadOnly.cpp:923 (Diff revision 1) > +already_AddRefed<nsStyleContext> > +KeyframeEffectReadOnly::GetTargetStyleContext() > +{ > + return GetTargetStyleContextByGetterFunc( > + // Need a lambda here since the forth argument of > + // nsComputedDOMStyle::GetStyleContextForElement is a default argument. > + [](Element* aElement, nsIAtom* aPseudo, nsIPresShell* aPresShell) { > + return nsComputedDOMStyle::GetStyleContextForElement(aElement, > + aPseudo, > + aPresShell); > + } > + ); > +} It seems like just adding a DoGetTargetStyleContext helper that takes a AnimationStyle::{Skip,Include} enum parameter would be simpler that adding templates and lambdas. Any reason to prefer this approach? ::: dom/animation/KeyframeEffectReadOnly.cpp:927 (Diff revision 1) > + > +already_AddRefed<nsStyleContext> > +KeyframeEffectReadOnly::GetTargetStyleContext() > +{ > + return GetTargetStyleContextByGetterFunc( > + // Need a lambda here since the forth argument of s/forth/fourth/

Comment on attachment 8826992 [details] Bug 1330190 - Part 6: Add MOZ_DIAGNOSTIC_ASSERT for mIsComposing. https://reviewboard.mozilla.org/r/104794/#review105518 I think the commit message should be something like: "Based on the other changesets in this series, we don't expect to be calling UpdateProperties() and ComposeStyle() from within ComposeStyle() itself. However, in case there is some scenario where that does stil occur, we leave the mIsComposing check in place and add an equivalent MOZ_DIAGNOSTIC_ASSERT so that we are alerted if this does occur on debug / Nightly / DevEdition builds, but handle it gracefully on beta or release builds." ::: dom/animation/KeyframeEffectReadOnly.cpp:276 (Diff revision 1) > > + MOZ_DIAGNOSTIC_ASSERT(!mIsComposingStyle, > + "Should not be called while processing ComposeStyle()"); Nit: Perhaps drop the blank line between MOZ_ASSERT and MOZ_DIAGNOSTIC_ASSERT to logically group them together? Or move this down together with the early return to logically group it with that--actually, that might be better.

Comment on attachment 8826991 [details] Bug 1330190 - Part 5: Get style context without all of animation data for base styles. https://reviewboard.mozilla.org/r/104792/#review105544 ::: dom/animation/KeyframeEffectReadOnly.cpp:923 (Diff revision 1) > +already_AddRefed<nsStyleContext> > +KeyframeEffectReadOnly::GetTargetStyleContext() > +{ > + return GetTargetStyleContextByGetterFunc( > + // Need a lambda here since the forth argument of > + // nsComputedDOMStyle::GetStyleContextForElement is a default argument. > + [](Element* aElement, nsIAtom* aPseudo, nsIPresShell* aPresShell) { > + return nsComputedDOMStyle::GetStyleContextForElement(aElement, > + aPseudo, > + aPresShell); > + } > + ); > +} No reason. I guess I was charmed with the magic of lambda. ;-)

Comment on attachment 8826992 [details] Bug 1330190 - Part 6: Add MOZ_DIAGNOSTIC_ASSERT for mIsComposing. https://reviewboard.mozilla.org/r/104794/#review105518 Thank you so much! I was confused that the beta channel has the same functionality of aurora channel again.

Comment on attachment 8826987 [details] Bug 1330190 - Part 1: Add nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation. https://reviewboard.mozilla.org/r/104784/#review105532 ::: layout/style/nsComputedDOMStyle.h:92 (Diff revision 1) > static already_AddRefed<nsStyleContext> > GetStyleContextForElement(mozilla::dom::Element* aElement, nsIAtom* aPseudo, > nsIPresShell* aPresShell, > StyleType aStyleType = eAll); > > + // Similar to the above but ignore animation rules. "ignoring" or "without". Also mention that it is StyleType::eAll, since this function doesn't give you the option. ::: layout/style/nsComputedDOMStyle.h:96 (Diff revision 1) > > + // Similar to the above but ignore animation rules. > + static already_AddRefed<nsStyleContext> > + GetStyleContextForElementWithoutAnimation(mozilla::dom::Element* aElement, > + nsIAtom* aPseudo, > + nsIPresShell* aPresShell); Nit: blank line below here. ::: layout/style/nsComputedDOMStyle.cpp:457 (Diff revision 1) > +enum AnimationFlags { > + eWithAnimation, > + eWithoutAnimation, > +}; > + > +template<AnimationFlags aAnimationFlag> I don't think it's worth making the AnimationFlags a template parameter, rather than a regular parameter. There's nothing performance critical here that would benefit from having two versions of DoGetStyleContextForElementNoFlush in the compiled code. ::: layout/style/nsComputedDOMStyle.cpp:458 (Diff revision 1) > + eWithAnimation, > + eWithoutAnimation, > +}; > + > +template<AnimationFlags aAnimationFlag> > +MOZ_STACK_CLASS class AutoSkipAnimationRules final { Nit: newline before "{". I'm not sure it matters, but it seems that most other uses of MOZ_STACK_CLASS put that right after the "class". ::: layout/style/nsComputedDOMStyle.cpp:476 (Diff revision 1) > + if (aPresContext->RestyleManager()->IsGecko()) { > + mRestyleManager = aPresContext->RestyleManager()->AsGecko(); > + > + mOldSkipAnimationRules = mRestyleManager->SkipAnimationRules(); > + mRestyleManager->SetSkipAnimationRules(true); > + } Can you add an NS_WARNING("stylo: can't skip animation rules yet") in an |else|? ::: layout/style/nsComputedDOMStyle.cpp:494 (Diff revision 1) > +}; > +} > + > +template<AnimationFlags AnimationFlag> > already_AddRefed<nsStyleContext> > -nsComputedDOMStyle::GetStyleContextForElementNoFlush(Element* aElement, > +DoGetStyleContextForElementNoFlush(Element* aElement, Maybe just make this a private method of nsComputedDOMStyle, so that we don't have to qualify a few things with "nsComputedDOMStyle::".

Comment on attachment 8826988 [details] Bug 1330190 - Part 2: Add a new function named ResolveStyleWithoutAnimation. https://reviewboard.mozilla.org/r/104786/#review105564 ::: layout/style/nsStyleSet.h:188 (Diff revision 1) > + // Similar to the above, but resolving style without all animation data in > + // the first place. > + already_AddRefed<nsStyleContext> > + ResolveStyleWithoutAnimation(mozilla::dom::Element* aTarget, > + nsStyleContext* aParentContext); I think it might be confusing that we have two functions named ResolveStyleWithoutAnimation here, but the meaning of the nsStyleContext parameter is different. Nearly all of the other ResolveStyleXXX functions use the nsStyleContext as the parent style. One function that doesn't is ResolveStyleByAddingRules, and I think the naming of the function suggests that we're doing something different. So, I'd like to rename the existing ResolveStyleWithoutAnimation to ResolveStyleByRemovingAnimation. Then the new function can remain as ResolveStyleWithoutAnimation. ::: layout/style/nsStyleSet.h:539 (Diff revision 1) > + template<AnimationFlags AnimationFlag> > + already_AddRefed<nsStyleContext> > + ResolveStyleForInternal(mozilla::dom::Element* aElement, > + nsStyleContext* aParentContext, > + TreeMatchContext& aTreeMatchContext); Here, too, I don't think we should make this a template argument. ::: layout/style/nsStyleSet.cpp:1804 (Diff revision 1) > + // Here we can call ResolveStyleForInternal() instead of > + // ResolveStyleWithReplacement() since we don't need all of animation rules > + // (CSS Animations, Transitions and script animations). That's because > + // EffectCompositor::GetAnimationRule() skips all of animations rules if > + // SkipAnimationRules flag is true. s/all of animation rules/any animation rules/

Comment on attachment 8826989 [details] Bug 1330190 - Part 3: Add ResolvePseudoElementStyleWithoutAnimation. ?heycam https://reviewboard.mozilla.org/r/104788/#review105566 ::: layout/style/nsStyleSet.h:553 (Diff revision 1) > + template<AnimationFlags AnimationFlag> > + already_AddRefed<nsStyleContext> > + ResolvePseudoElementStyleInternal(mozilla::dom::Element* aParentElement, Change template argument to a regular argument.

Comment on attachment 8826990 [details] Bug 1330190 - Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation(). https://reviewboard.mozilla.org/r/104790/#review105570

Comment on attachment 8826990 [details] Bug 1330190 - Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation(). https://reviewboard.mozilla.org/r/104790/#review105572 (Clicked this one a bit too fast...)

Comment on attachment 8826990 [details] Bug 1330190 - Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation(). https://reviewboard.mozilla.org/r/104790/#review105582 ::: layout/style/nsComputedDOMStyle.cpp:500 (Diff revision 1) > +already_AddRefed<nsStyleContext> > +StyleResolver<eWithoutAnimation>::Resolve(StyleSetHandle aStyleSet, > + Element* aElement, > + CSSPseudoElementType aType, > + nsStyleContext* aParentContext, > + nsComputedDOMStyle::StyleType aStyleType, > + bool aInDocWithShell) I think it's a bit strange to have this template specialization that takes aStyleType but does nothing with it. It might be clearer if these are two separate functions. If not, then at least assert inside this function that aStyleType is eAll.

Comment on attachment 8826988 [details] Bug 1330190 - Part 2: Add a new function named ResolveStyleWithoutAnimation. https://reviewboard.mozilla.org/r/104786/#review105584 ::: layout/style/nsStyleSet.h:188 (Diff revision 1) > + // Similar to the above, but resolving style without all animation data in > + // the first place. > + already_AddRefed<nsStyleContext> > + ResolveStyleWithoutAnimation(mozilla::dom::Element* aTarget, > + nsStyleContext* aParentContext); Great! ResolveStyleByRemovingAnimation sounds really nice, it matches to what that function does! Thanks!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > ::: layout/style/nsStyleSet.h:188 > (Diff revision 1) > > + // Similar to the above, but resolving style without all animation data in > > + // the first place. > > + already_AddRefed<nsStyleContext> > > + ResolveStyleWithoutAnimation(mozilla::dom::Element* aTarget, > > + nsStyleContext* aParentContext); > > Great! ResolveStyleByRemovingAnimation sounds really nice, it matches to > what that function does! Thanks! Hmm, MozReview is really hard to use for me.. (In reply to Cameron McCormack (:heycam) from comment #22) > ::: layout/style/nsStyleSet.h:188 > (Diff revision 1) > > + // Similar to the above, but resolving style without all animation data in > > + // the first place. > > + already_AddRefed<nsStyleContext> > > + ResolveStyleWithoutAnimation(mozilla::dom::Element* aTarget, > > + nsStyleContext* aParentContext); > > I think it might be confusing that we have two functions named > ResolveStyleWithoutAnimation here, but the meaning of the nsStyleContext > parameter is different. Nearly all of the other ResolveStyleXXX functions > use the nsStyleContext as the parent style. One function that doesn't is > ResolveStyleByAddingRules, and I think the naming of the function suggests > that we're doing something different. > > So, I'd like to rename the existing ResolveStyleWithoutAnimation to > ResolveStyleByRemovingAnimation. Then the new function can remain as > ResolveStyleWithoutAnimation. I wanted to reply to this comment.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fd18ab48f5c3 Part 1: Add nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation. r=heycam https://hg.mozilla.org/integration/autoland/rev/b418519ea4d0 Part 2: Add a new function named ResolveStyleWithoutAnimation. r=heycam https://hg.mozilla.org/integration/autoland/rev/69a81a6bb0bd Part 3: Add ResolvePseudoElementStyleWithoutAnimation. ?heycam r=heycam https://hg.mozilla.org/integration/autoland/rev/0a9984a0cc54 Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation(). r=heycam https://hg.mozilla.org/integration/autoland/rev/a2532fa6577d Part 5: Get style context without all of animation data for base styles. r=birtles https://hg.mozilla.org/integration/autoland/rev/8f52299c4c63 Part 6: Add MOZ_DIAGNOSTIC_ASSERT for mIsComposing. r=birtles

https://hg.mozilla.org/mozilla-central/rev/fd18ab48f5c3 https://hg.mozilla.org/mozilla-central/rev/b418519ea4d0 https://hg.mozilla.org/mozilla-central/rev/69a81a6bb0bd https://hg.mozilla.org/mozilla-central/rev/0a9984a0cc54 https://hg.mozilla.org/mozilla-central/rev/a2532fa6577d https://hg.mozilla.org/mozilla-central/rev/8f52299c4c63